[YUNIKORN-2933] Don't add duplicated taskGroup to app #928
Conversation
pbacsko
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #928 +/- ##
==========================================
- Coverage 68.62% 68.12% -0.51%
==========================================
Files 70 70
Lines 7675 9219 +1544
==========================================
+ Hits 5267 6280 +1013
- Misses 2202 2732 +530
- Partials 206 207 +1 ☔ View full report in Codecov by Sentry. |
pkg/cache/application.go
Outdated
| @@ -80,7 +81,7 @@ func NewApplication(appID, queueName, user string, groups []string, tags map[str | |||
| taskMap: taskMap, | |||
| tags: tags, | |||
| sm: newAppState(), | |||
| taskGroups: make([]TaskGroup, 0), | |||
| taskGroups: taskGroups, | |||
There was a problem hiding this comment.
nit:
taskGroups: make(map[string]*TaskGroup)
pkg/cache/application.go
Outdated
| @@ -71,6 +71,7 @@ func (app *Application) String() string { | |||
|
|
|||
| func NewApplication(appID, queueName, user string, groups []string, tags map[string]string, scheduler api.SchedulerAPI) *Application { | |||
| taskMap := make(map[string]*Task) | |||
| taskGroups := make(map[string]*TaskGroup) | |||
There was a problem hiding this comment.
Does it have to be a pointer? Probably not.
There was a problem hiding this comment.
Does it have to be a pointer? Probably not.
Thank you for your reply, I agree with you.
pkg/cache/application.go
Outdated
| for _, taskGroup := range app.taskGroups { | ||
| app.placeholderAsk = common.Add(app.placeholderAsk, common.GetTGResource(taskGroup.MinResource, int64(taskGroup.MinMember))) | ||
| for _, tg := range taskGroups { | ||
| taskGroup := tg |
There was a problem hiding this comment.
If the map is string[TaskGroup] then this is not needed I believe.
There was a problem hiding this comment.
If the map is
string[TaskGroup]then this is not needed I believe.
Yes, if the map is string[TaskGroup], it really doesn 't need to be done here.
| log.Log(log.ShimCacheApplication).Warn("duplicate task-group within the task-groups", | ||
| zap.String("appID", app.applicationID), | ||
| zap.String("groupName", taskGroup.Name)) |
There was a problem hiding this comment.
Make sure you have an unit test which covers this branch.
There was a problem hiding this comment.
Make sure you have an unit test which covers this branch.
Well yeah, that 's a good addition. I will provide the unit test which covers this branch.
pkg/cache/application.go
Outdated
|
|
||
| taskGroups := make([]TaskGroup, 0) | ||
| if len(app.taskGroups) > 0 { | ||
| for _, taskGroup := range app.taskGroups { | ||
| taskGroups = append(taskGroups, *taskGroup) | ||
| } | ||
| } | ||
| return taskGroups |
There was a problem hiding this comment.
nit:
if len(app.taskGroups) > 0 {
taskGroups := make([]TaskGroup, 0, len(app.taskGroups))
for _, taskGroup := range app.taskGroups {
taskGroups := append(taskGroups , *taskGroup)
}
return taskGroups
}
return nil
|
We cannot just eat the duplicates and not provide feedback to the customer via an event or something on the original pod. Without specifying and documenting the behaviour I am against making this change. |
Thank you for your reply. In PR, I do ignore prompting submitter about our behavior, which is really important. I will add this part of the description in the code and feedback to the submitter via an event on the original pod. |
+1 I forgot to say this. And also let's add some info to the docs on the site about how this is intended to work and warn the users that they should pay attention to the task group definition. We might even reject pods with incorrect taskgroup data. |
|
According to YUNIKORN-2933 discussion, always taking the first task-group doesn 't seem like a good choice, so close this PR. |
What is this PR for?
I think that before properly handling ph tasks with different resource specifications within the same task group, it is necessary to avoid having two task-group with the same name but different resource specifications in the app.
What type of PR is it?
Todos
What is the Jira issue?
Issue associated with this PR：https://issues.apache.org/jira/browse/YUNIKORN-2933
How should this be tested?
kubectl -n [namespace Name] get pod -w)Screenshots (if appropriate)
Questions: